Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for ".nomedia" paths and deletes in Viewer #1417

Closed
wants to merge 3 commits into from

Conversation

jkellerer
Copy link

@jkellerer jkellerer commented Oct 24, 2022

This PR is a collection of 3 2 fixes in 2 commits:

  • Source URL could be invalid and this broke sidebar, download and file-editing in the Viewer. The URL derives from filename but this was not consistently built (sometimes with, sometimes without prefix). This change ensures filename is consistent and a valid URL can always be built - moved to Fix 'source' URL in viewer #1419.
  • $store.appendFiles may filter files in .nomedia paths but the callers of appendFiles didn't evaluate whether the method really appended all files, leading to broken references. This change ensures that only files that were really appended will be considered.
  • A Delete File action inside the Viewer was not propagated to the Photos app (the view still contained deleted files and caused failures when trying to get the file from the server).

Hope this found useful. Had only fixed the most serious problems I found when trying the app.
Feel free to reject or accept the PR or individual commits of it.

@szaimen szaimen added the 3. to review Waiting for reviews label Oct 24, 2022
@szaimen szaimen added this to the Nextcloud 26 milestone Oct 24, 2022
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, the Fix 'source' URL in viewer part seems far too complicated.
I have plans to fix this for 26. Until then, I would like to keep the code simple.

Please strip this out from this PR and open a dedicated one for it so we can discuss this separately :)

@jkellerer jkellerer force-pushed the fix/nomedia-and-viewer branch from 0294851 to 96bae5a Compare October 25, 2022 08:34
@jkellerer jkellerer changed the title Fix for ".nomedia" and source URL in Viewer Fix for ".nomedia" paths and deletes in Viewer Oct 25, 2022
@jkellerer jkellerer force-pushed the fix/nomedia-and-viewer branch from 96bae5a to da7e5eb Compare October 25, 2022 09:09
@szaimen szaimen requested a review from skjnldsv October 25, 2022 21:26
src/store/files.js Outdated Show resolved Hide resolved
Signed-off-by: Juergen Kellerer <[email protected]>
@simonspa
Copy link

simonspa commented Feb 9, 2023

@artonge anything missing for this pr? Looks complete and has an approval.

If Cypress fail is related is unclear since the logs expired

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants